Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes spurious error messages in tests #1400

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Conversation

aeisenberg
Copy link
Contributor

Previously, isAnalyzingDefaultBranch was failing because there are some missing env vars: GITHUB_SHA, GITHUB_REF, and GITHUB_EVENT_PATH. Also, checkout_path is missing as an input.

Rather than trying to set them to mock values, which would require setting the paths to existing paths in the file system, I chose to stub the entire function. I think this is fine since the point of the test is to check the ram and threads values, not testing the isAnalyzingDefaultBranch function.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@aeisenberg aeisenberg requested a review from a team as a code owner November 28, 2022 22:08
angelapwen
angelapwen previously approved these changes Nov 29, 2022
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this!!

Just curious, were the tests passing in CI because GITHUB_SHA, GITHUB_REF, and GITHUB_EVENT_PATH were set by default in CI, just not locally?

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I checked this out locally and src/analyze-action-input.test.ts is still failing. Thankfully we can just apply the same fix to get that passing. Once we've done that all the tests succeed locally.

@aeisenberg
Copy link
Contributor Author

aeisenberg commented Nov 29, 2022

Just curious, were the tests passing in CI because GITHUB_SHA, GITHUB_REF, and GITHUB_EVENT_PATH were set by default in CI, just not locally?

The tests are still "passing" locally (ie- the test process doesn't fail and all 333 tests are marked as passing). It's just that there is a subprocess command that gets invoked and fails. The error is swallowed by the action (I think that's the behaviour we want).

And most likely, we're not seeing the same messages on CI since all of the associated env vars are in place and the workflow file is where we expect it to be.

@aeisenberg
Copy link
Contributor Author

Very nice! I checked this out locally and src/analyze-action-input.test.ts is still failing. Thankfully we can just apply the same fix to get that passing. Once we've done that all the tests succeed locally.

Right...forgot about the second one. Let me get that in, too.

Previously, `isAnalyzingDefaultBranch` was failing because there are
some missing env vars: `GITHUB_SHA`, `GITHUB_REF`, and
`GITHUB_EVENT_PATH`. Also, `checkout_path` is missing as an input.

Rather than trying to set them to mock values, which would require
setting the paths to existing paths in the file system, I chose to stub
the entire function. I think this is fine since the point of the test
is to check the ram and threads values, not testing the
`isAnalyzingDefaultBranch` function.
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I pulled down the branch and ran to confirm this time as well 👍

@aeisenberg aeisenberg merged commit a631f4b into main Nov 30, 2022
@aeisenberg aeisenberg deleted the aeisenberg/fix-test-error branch November 30, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants